Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: split cli methods to easy testing #3

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

fzipi
Copy link
Member

@fzipi fzipi commented Jan 21, 2025

what

  • refactor main to simpler methods
  • apply black formatting
  • include github actions specific library
  • apply grouping in output

why

  • easy testing and readability
  • use additional gha features

@fzipi fzipi requested a review from airween January 21, 2025 12:44
src/crs_linter/cli.py Outdated Show resolved Hide resolved
src/crs_linter/cli.py Outdated Show resolved Hide resolved
src/crs_linter/cli.py Outdated Show resolved Hide resolved
src/crs_linter/cli.py Outdated Show resolved Hide resolved
src/crs_linter/cli.py Outdated Show resolved Hide resolved
src/crs_linter/cli.py Outdated Show resolved Hide resolved
src/crs_linter/cli.py Outdated Show resolved Hide resolved
src/crs_linter/linter.py Outdated Show resolved Hide resolved
fzipi and others added 8 commits January 22, 2025 09:40
Signed-off-by: Felipe Zipitria <[email protected]>
Signed-off-by: Felipe Zipitria <[email protected]>
Signed-off-by: Felipe Zipitria <[email protected]>
Signed-off-by: Felipe Zipitria <[email protected]>
Signed-off-by: Felipe Zipitria <[email protected]>
Signed-off-by: Felipe Zipitria <[email protected]>
Signed-off-by: Felipe Zipitria <[email protected]>
@fzipi fzipi force-pushed the refactor/split-cli-methods branch from 8b1991a to 4477d11 Compare February 3, 2025 16:41
Signed-off-by: Felipe Zipitria <[email protected]>
@fzipi fzipi force-pushed the refactor/split-cli-methods branch from 4477d11 to 646208c Compare February 8, 2025 21:52
fzipi added 3 commits February 9, 2025 10:56
Signed-off-by: Felipe Zipitria <[email protected]>
Signed-off-by: Felipe Zipitria <[email protected]>
Signed-off-by: Felipe Zipitria <[email protected]>
@fzipi fzipi force-pushed the refactor/split-cli-methods branch from 4db6a91 to 3b4b403 Compare February 9, 2025 14:33
fzipi added 2 commits February 9, 2025 11:42
Signed-off-by: Felipe Zipitria <[email protected]>
@fzipi fzipi requested a review from theseion February 9, 2025 14:44
.github/workflows/test.yml Outdated Show resolved Hide resolved
src/crs_linter/cli.py Outdated Show resolved Hide resolved
src/crs_linter/cli.py Outdated Show resolved Hide resolved
src/crs_linter/cli.py Outdated Show resolved Hide resolved
src/crs_linter/cli.py Outdated Show resolved Hide resolved
src/crs_linter/linter.py Outdated Show resolved Hide resolved
src/crs_linter/linter.py Outdated Show resolved Hide resolved
src/crs_linter/linter.py Outdated Show resolved Hide resolved
src/crs_linter/linter.py Outdated Show resolved Hide resolved
src/crs_linter/linter.py Outdated Show resolved Hide resolved
@fzipi
Copy link
Member Author

fzipi commented Feb 9, 2025

Ugh, the code review changes broke it. I'll try to fix.

Copy link

@airween airween left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a first review I found only one thing (see above), but I'd like to run few tests (after we cleared my review).


if "operator_argument" in d:
oparg = re.findall(r"%\{(tx.[^%]*)\}", d['operator_argument'], re.I)
oparg = re.findall(r"%\{(tx.[^%]*)}", d["operator_argument"], re.I)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you removed the \ from the front of the close bracket. Is that accidentally or on purpose?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless, the expression are equivalent. In python even the opening brace does not need to be escaped, unless it's umbiguous (e.g., looks like a range expression).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, but there are many other places (eg. here, here or here) where the syntax follows the "old" format. I think it would be nice if we used the same syntax (and personally I prefer the old way, that's much clearer).

Copy link
Member Author

@fzipi fzipi Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Python regular expressions, special characters that need to be escaped outside of character classes include: *, +, ?, {, (, ), ^, $, |, and \. Inside character classes, you need to escape ], -, ^, and \ if they are used in a way that could confuse the regex parser.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, formatting should be consistent.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, formatting should be consistent.

Yeah, this is what I noticed.

@fzipi fzipi force-pushed the refactor/split-cli-methods branch 2 times, most recently from 2a8ba08 to 0500fea Compare February 10, 2025 21:09
@fzipi fzipi force-pushed the refactor/split-cli-methods branch from 0500fea to 025f053 Compare February 10, 2025 21:17
Signed-off-by: Felipe Zipitria <[email protected]>
@fzipi fzipi force-pushed the refactor/split-cli-methods branch from 025f053 to 3f10697 Compare February 10, 2025 21:25
"endLine": line1 + line2,
"message": "an indentation error has found",
}
errmsgf(e)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function errmsgf() is undefined.

print("::debug::%s" % (msg))
else:
print(msg)
from crs_linter.logger import Logger
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a minimal modification we could make this tool more flexible, I mean if someone wants to avoid to use uv, then they can use native was.

diff --git a/src/crs_linter/cli.py b/src/crs_linter/cli.py
index 5469cf7..5580c21 100755
--- a/src/crs_linter/cli.py
+++ b/src/crs_linter/cli.py
@@ -10,8 +10,14 @@ import re
 from dulwich.contrib.release_robot import get_current_version, get_recent_tags
 from semver import Version
 
-from crs_linter.linter import Check
-from crs_linter.logger import Logger
+try:
+    from linter import Check
+except:
+    from crs_linter.linter import Check
+try:
+    from logger import Logger
+except:
+    from crs_linter.logger import Logger
 
 
 def remove_comments(data):

And anyone can run from that directory with command ./cli.py ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know what you mean. People will still need to install prerequisites, right?

How is people going to install requirements (we will provide the option, of course)? If you want to use native, you probably also want to use virtualenv, right?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to use native, you probably also want to use virtualenv, right?

Yes, of course.

choices=["native", "github"],
required=False,
)
parser.add_argument(
Copy link

@airween airween Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This argument isn't documented in README.md (even it's mandatory).

Edit: I meant the -d DIRECTORY

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Now being a tool that lives outside the coreruleset directory, it needs it to properly catch the git labels.

It might not be needed if we are passing the proper version 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants